The design here is sound — FnOnce + Send + 'static is the right bound (FnOnce matches the builder being consumed by build_consensus, Send + 'static are required for async node startup), the manual Debug impl is the only valid approach for a Box<dyn FnOnce> field, and the type alias makes the otherwise-unreadable bound navigable. A few points worth addressing before merge:

1. Removing Clone and Copy is a semver-breaking change

// before
#[derive(Debug, Default, Clone, Copy)]
pub struct ArcConsensusBuilder { ... }

// after
#[derive(Default)]
pub struct ArcConsensusBuilder { ... }

Dropping Clone and Copy from a pub struct is a breaking change under semver — any downstream code that clones or copies the builder will no longer compile. The removal is correct (a Box<dyn FnOnce> is neither Clone nor Copy), but if ArcConsensusBuilder is part of a released public API this should be called out explicitly (changelog entry, major version bump, or at minimum a comment in the PR description). If the struct was previously only compiled internally and never shipped in a release, worth confirming that and noting it here.

2. Double with_consensus_modifier silently discards the first modifier

pub fn with_consensus_modifier<F>(mut self, modify_consensus: F) -> Self {
    self.modify_consensus = Some(Box::new(modify_consensus));
    self
}

Calling this twice replaces the existing modifier with no warning. For a direct-construction call-site this is probably fine, but if ArcConsensusBuilder ever flows through a plugin or component layer where multiple parties might independently call with_consensus_modifier, the first modifier disappears silently. Two low-cost mitigations:

• Doc comment: "Calling this more than once replaces the previous modifier. To apply multiple transformations, compose them into a single closure before passing."
• Debug assertion: debug_assert!(self.modify_consensus.is_none(), "consensus modifier already set — previous modifier will be discarded");

The debug assertion costs nothing in release builds and will catch accidental double-sets during development.

3. Test exercises a private method directly

let modified = ArcConsensusBuilder::default()
    .with_consensus_modifier(...)
    .apply_consensus_modifier(consensus.clone())  // private
    .expect("...");

This is valid Rust (child modules can access private items from the parent module), but it tests the private method rather than the observable public contract. If apply_consensus_modifier is ever refactored or inlined, the test will need to change even if the public behaviour is unchanged. A test against build_consensus would be more robust, though that requires constructing a BuilderContext — if that's too heavyweight, an alternative is to make apply_consensus_modifier pub(crate) and document it as an internal seam, making the visibility explicit rather than implicit.

Future consideration — modifier composability

The FnOnce bound means the API supports exactly one modifier per builder instance. If this becomes an extension point for downstream node customisations (e.g., different teams layering monitoring, validation, or replay-protection wrappers), callers will have to manually compose closures themselves. Not a blocker for this PR, but worth a note in the doc comment if external composability is a goal:

// callers must compose modifiers themselves:
.with_consensus_modifier(|c| {
    let c = modifier_a(c)?;
    modifier_b(c)
})

Points 2 and 3 from the earlier review are addressed well — the debug_assert! + doc comment is exactly the right approach for point 2, and pub(crate) with a doc comment is the right fix for the test-seam visibility on apply_consensus_modifier.

One new issue in this commit, and one still-open point:

New: #[should_panic] test fails under cargo test --release

#[test]
#[should_panic(expected = "consensus modifier already set")]
fn arc_consensus_builder_panics_on_double_modifier_in_debug() {
    let _builder = ArcConsensusBuilder::default()
        .with_consensus_modifier(Ok)
        .with_consensus_modifier(Ok);
}

debug_assert! is compiled out entirely in release mode. In a cargo test --release run the double-set silently succeeds, no panic fires, and #[should_panic] sees a test that returned normally rather than panicking — which it treats as a test failure. The test name contains _in_debug as a hint, but that doesn't protect the CI step.

Fix: gate the test on debug_assertions:

#[test]
#[cfg(debug_assertions)]
#[should_panic(expected = "consensus modifier already set")]
fn arc_consensus_builder_panics_on_double_modifier_in_debug() {
    let _builder = ArcConsensusBuilder::default()
        .with_consensus_modifier(Ok)
        .with_consensus_modifier(Ok);
}

#[cfg(debug_assertions)] ensures the test is compiled and run only when debug assertions are active, making the #[should_panic] expectation consistent with the runtime behaviour.

Still open: Clone and Copy removal is a semver-breaking change

From the earlier comment — removing both derives from a pub struct breaks any downstream code that clones or copies the builder. Whether that matters depends on whether ArcConsensusBuilder was shipped in a release. If it was internal-only before this PR that's worth confirming in the description; if it was released, a changelog/semver note is needed.

#[cfg(debug_assertions)] is the correct fix — the test now only compiles and runs when assertions are active, so #[should_panic] and debug_assert! are always in sync regardless of whether CI runs cargo test or cargo test --release.

The only outstanding point from this review thread is the semver question on Clone + Copy removal (raised in the first comment). Everything else is resolved.

The semver question is now fully addressed — the [Unreleased] entry in both BREAKING_CHANGES.md and CHANGELOG.md correctly documents the Clone/Copy removal, names v0.7.2 as the last release that had them, and gives a clear migration path. That closes the last outstanding point from this review thread.

One minor thing worth noting before the release is cut: the [Unreleased] entry doesn't indicate what the next version number will be. Downstream crates pinning ~0.7 or ^0.7 will be broken silently if this ships as v0.7.3 — they'd expect a patch-compatible release. For pre-1.0 crates the convention varies, but if Arc follows semver for its Rust API consumers this should land as v0.8.0 (minor bump signals breaking changes in the 0.x range). Worth confirming the target version in the release notes so integrators know which range constraint to update.

Everything else in the PR is resolved. No further issues from this review thread.